Add set_bit to BooleanBufferBuilder to allow mutating bit in index #383
Conversation
|
I added some details on the ASF Slack. I think the change is fine, but only the tests should be fixed. Maybe it makes sense to expose |
|
FYI @tustvold |
|
Thanks @boazberman ! Here is the Slack link @Dandandan is referring to: https://the-asf.slack.com/archives/C01QUFS30TD/p1622403410176000 FWIW @tustvold implemented some version of this logic in https://github.com/influxdata/influxdb_iox/blob/main/arrow_util/src/bitset.rs in case you want to have a friendly look (and perhaps we can eventually use |
|
Fix the tests, could you review? @Dandandan @alamb ? Also, @alamb I noticed that in https://github.com/influxdata/influxdb_iox/blob/main/arrow_util/src/bitset.rs#L87 is differ from what is done here: // influx_iox
data[i >> 3] |= 1 << (i & 7) vs // arrow-rs
const BIT_MASK: [u8; 8] = [1, 2, 4, 8, 16, 32, 64, 128];
// ...
data[i >> 3] |= BIT_MASK[i & 7];which I haven't tested but might have performance implications (?) |
arrow/src/array/builder.rs
Outdated
| @@ -302,6 +302,15 @@ impl BooleanBufferBuilder { | |||
| self.len | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub fn set_bit(&mut self, index: usize, bit: bool) { | |||
There was a problem hiding this comment.
| pub fn set_bit(&mut self, index: usize, bit: bool) { | |
| pub fn set_bit(&mut self, index: usize, v: bool) { |
(as called in other methods)
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
==========================================
+ Coverage 82.62% 82.65% +0.02%
==========================================
Files 162 162
Lines 44479 44542 +63
==========================================
+ Hits 36751 36814 +63
Misses 7728 7728
Continue to review full report at Codecov.
|
Hopefully the compiler can figure it out -- I think the way you have it is fine until we get some sort of profiling / performance measurements that show it isn't. 👍 |
There was a problem hiding this comment.
Looks great @boazberman -- thank you!
Would it be possible to fix the rust clippy CI check? Then I think this is ready to merge in.
🥇 for tests 👍
|
@alamb fixed clippy failures |
|
hey, what is blocking this from being merged? @Dandandan @alamb |
Merged! Thank you 🚀 |
Lack of time :) Sorry about that @boaz-codota -- thanks for following up and thanks for merging @Dandandan 👍 |
) * Add set_bit to BooleanBufferBuilder to allow mutating bits in the builder * Fix tests * Update builder.rs * Update builder.rs * Fix clippy failures Co-authored-by: Boaz Berman <[email protected]>
) (#431) * Add set_bit to BooleanBufferBuilder to allow mutating bits in the builder * Fix tests * Update builder.rs * Update builder.rs * Fix clippy failures Co-authored-by: Boaz Berman <[email protected]> Co-authored-by: Boaz <[email protected]> Co-authored-by: Boaz Berman <[email protected]>
Which issue does this PR close?
Relates to apache/datafusion#240 & apache/datafusion#342
Rationale for this change
BooleanBufferBuilderbehaves as a bitmap. My intention is to extend its API to allow mutating bits already set, to make it's API closer to thebitveccrate and useable for my use-case inarrow-datafusion.What changes are included in this PR?
This PR includes the implementation and some tests that currently does not pass. I'm making the PR public to get help from the community, as I'm stuck.
Are there any user-facing changes?
There is a new public method
set_bitadded toBooleanBufferBuilder.